Skip to content
This repository was archived by the owner on Jan 24, 2026. It is now read-only.

Rule action persistence policies#8

Merged
rcahoon merged 7 commits into
mf3-devfrom
rcahoon/mf3-rule-persistence
Jan 13, 2025
Merged

Rule action persistence policies#8
rcahoon merged 7 commits into
mf3-devfrom
rcahoon/mf3-rule-persistence

Conversation

@rcahoon
Copy link
Copy Markdown
Member

@rcahoon rcahoon commented Jan 10, 2025

Description

Allow the user to specify how the RuleEngine should handle a Rule's action when the action completes or the Rule stops triggering.

Original PR from 2024 repo: Team766/2024#146

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Be detailed so that your code reviewer can understand exactly how much and what kinds of testing were done, and which might still be worthwhile to do.

  • Unit tests: Units tests for new functionality are included in this PR
  • Simulator testing: [Add your description here]
  • On-robot bench testing: [Add your description here]
  • On-robot field testing: [Add your description here]

…urn a Rule that can be modified further, eg with a finishedTriggeringProcedure.
this prevents accidental modification of rules after the containing ruleengine starts getting used.

also switch to a LinkedHashMap for storing rules and tweak some of the methods used in unit tests.
@rcahoon rcahoon requested a review from dejabot January 10, 2025 05:13
@rcahoon rcahoon force-pushed the rcahoon/mf3-rule-persistence branch from f8d72f3 to 5a4a3ae Compare January 13, 2025 03:22
Comment thread .vscode/settings.json Outdated
Comment thread src/main/java/com/team766/framework3/Rule.java

protected Rule addRule(String name, BooleanSupplier condition, Supplier<Procedure> action) {
Rule rule = new Rule(name, condition, action);
protected Rule addRule(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we hold off on merging this PR until the RuleEngine API simplification goes through?

I think that would remove the need for a lot of these overloaded versions of addRule().

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

n/m about the first paragraph since this is based on that other PR.. do you think we could reduce the overloading and let devs more simply specify a withRulePersistence if they want something other than ONCE_AND_HOLD (if we want that to be the default)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

withRulePersistence seems weird to me, because withFinishedTriggeringProcedure is specifying a whole other action, whereas withRulePersistence would be modifying just the onTriggering action. i.e. it would be possible to say addRule(...).withFinishedTriggeringProcedure(...).withRulePersistence(...), which seems misleading.

does standard Java style discourage the use of overloads?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not at all - there are just a lot of variants. it's too bad java doesn't have named and optoinal parameters. :)

just looked - there are articles like this that also speak to alternatives to method overloading when there are a larger number of parameters.

I'm fine either way - was just wondering how to keep this scalable esp if and as we do add more parameters.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a good point. we can discuss again if we add more parameters

Base automatically changed from mf3-simplify-rule-engine-apis to mf3-dev January 13, 2025 07:14
@rcahoon rcahoon merged commit bc8a3ff into mf3-dev Jan 13, 2025
@rcahoon rcahoon deleted the rcahoon/mf3-rule-persistence branch January 13, 2025 08:04
rajitzg pushed a commit that referenced this pull request Jan 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants